-
Notifications
You must be signed in to change notification settings - Fork 150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow filtering samples by compound expressions including multiple scorers #1073
base: main
Are you sure you want to change the base?
Conversation
This is looking really great to me, and I love that we'll be able to support much more robust filtering! I believe that the right autocomplete experience can make this nearly as easy to use as the simple selector. I have some suggestions to get there:
Don't mean to flood with feedback, but this is definitely getting there and I'm looking forward to merging it!! |
Thank for the feedback! Implemented all suggestions, please take a look. |
(These check failures are not related to this PR and are related to ruff dependency version changes. They are now fixed on main so if you rebase against main they should go away - sorry!) |
Uses filtrex to support compound expressions that allow to filter samples by multiple scores at a time.
… likely to continue
Version 18.1.1 (20619.2.8.11.12). I only see it once I make the expression long and scroll with the mouse... |
One other question - rather than show the green feedback treatment once an expression is complete, maybe we should just apply the expression at that point since we know it will work? It would still result in some changes to filtering in cases where the filters didn't narrow the set (e.g or) but I think that would be worth getting an even smoother experience. |
My Safari version is slightly different (Version 18.2 (20620.1.16.11.8)), don't know if it's related or not. To be honest, debugging this kind of failure without being able to reproduce it would be quite hard. I decided to just remove the scroll bar. It's quite unusual for single-line text inputs to have scroll bars anyway. |
Agreed. Applying the filter expression immediately but only when it's valid seems like a good approach. Changed the behavior and added color-coding, which is hopefully noticeable enough to make the current state clear, but not so much as to be distracting. |
This is a great improvement over our current filtering. Nits:
I haven't looked closely at the code itself - LMK if you think that is ready to go and I can take a look (or just ping me whenever you think its good to go). |
This is a reincarnation of #911
This PR contains:
What is the current behavior?
Samples can be filtered based on simple conditions including one scorer.
What is the new behavior?
Samples can be filtered by compound expressions like
result == "C" and steps <= 10
. Additionally, samples can be filtered based on input and target texts.result == "C"
.Auxiliary changes:
score.foo
vsother_score.foo
.Does this PR introduce a breaking change?
No.
Other information:
Next steps:
This PR is a work in progress. In particular, I remember that @dragonstyle suggested to only apply filter on Enter. This haven't been done yet. I'm also still figuring out some corner cases with different score types. Still, @dragonstyle, if you could take a look at the current state I would appreciate your feedback. Do you this is moving the right direction?